feat: organization avatar upload (set/update/remove)#769
Open
softmarshmallow wants to merge 7 commits into
Open
feat: organization avatar upload (set/update/remove)#769softmarshmallow wants to merge 7 commits into
softmarshmallow wants to merge 7 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
6 Skipped Deployments
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
WalkthroughProfile updates moved from an API route to a Next.js Server Action that handles auth, RLS authorization, text-field updates, avatar upload/removal to an ChangesOrganization Profile Update with Avatar Management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2908c13a97
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import { createClient, service_role } from "@/lib/supabase/server"; | ||
| import { revalidatePath } from "next/cache"; | ||
|
|
||
| const AVATAR_MAX_BYTES = 2 * 1024 * 1024; // ~2MB |
There was a problem hiding this comment.
For avatars between 1MB and 2MB, this limit says the file is accepted, but the form now posts through a Server Action and I checked editor/next.config.ts: it does not configure experimental.serverActions.bodySizeLimit, so Next rejects the request at its default 1MB limit before updateOrganizationProfile runs. Either lower this validation to the framework limit or raise the Next config; otherwise users choosing a “valid” 1–2MB image get a framework 413 instead of an avatar update.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@editor/app/`(site)/organizations/[organization_name]/settings/profile/actions.ts:
- Around line 57-63: The current check relies only on the client-provided
avatar.type (and later forwards it as contentType), so add server-side MIME
sniffing: read the first bytes via await avatar.arrayBuffer(), inspect magic
bytes/signature to detect actual file type (e.g., PNG/JPEG/GIF signatures) and
verify it matches an allowed type from AVATAR_ACCEPTED_TYPES and
AVATAR_MAX_BYTES; if the sniffed type and declared avatar.type disagree or the
signature is not an accepted image type, throw an error and do not forward the
client contentType to the upload — instead set the upload contentType based on
the sniffed/validated type before storing to the avatars bucket.
- Around line 86-95: The current update uses String(display_name)/String(email)
which will store the literal "null" if formData.get returned null; change to
explicitly validate and guard required fields before calling
client.from("organization").update: ensure display_name and email are non-null
(e.g., if (!display_name || !email) return an error/throw or set update to
abort) and then use the same guarded conversion pattern as description/blog
(display_name ? String(display_name) : undefined and email ? String(email) :
undefined) so you never call String(null); update the code around the
client.from("organization").update call and the variables display_name/email
handling to perform this check and early return.
In
`@editor/app/`(site)/organizations/[organization_name]/settings/profile/avatar-field.tsx:
- Around line 28-39: The code currently creates blob URLs in onPick via
URL.createObjectURL and never revokes them; update onPick to revoke any existing
preview URL before setting a new one (use the current preview state), update
onRemove to revoke the current preview URL before clearing it, and add a
useEffect cleanup that revokes the preview URL on unmount; reference the onPick
and onRemove handlers and the preview state variable so you revoke URL(s)
whenever preview changes or component unmounts.
In
`@editor/app/`(site)/organizations/[organization_name]/settings/profile/page.tsx:
- Around line 69-72: OrganizationAvatarField is being passed data.display_name
which can be null, but its display_name prop is typed as string; update the prop
contract or pass a safe value. Either change OrganizationAvatarField's prop type
for display_name to accept string | null | undefined (and keep its internal
checks like display_name?.charAt(0)), or coerce at the callsite by passing
display_name={data.display_name ?? undefined}. Update the TS type for
OrganizationAvatarField (and any related props/interfaces) or adjust this call
accordingly so types align.
- Around line 61-65: The profile form used by updateOrganizationProfile is
missing multipart encoding, so avatar File uploads won’t be sent correctly.
Update the form in profile/page.tsx by adding encType="multipart/form-data" to
the form element that wraps the avatar inputs, keeping the existing id and
action binding intact, and verify upload/removal flows still work through the
updateOrganizationProfile server action.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8e95bd25-59da-4454-860e-92a26f5e99b8
📒 Files selected for processing (5)
editor/app/(api)/private/accounts/organizations/[org]/profile/route.tseditor/app/(site)/organizations/[organization_name]/settings/profile/__tests__/actions.test.tseditor/app/(site)/organizations/[organization_name]/settings/profile/actions.tseditor/app/(site)/organizations/[organization_name]/settings/profile/avatar-field.tsxeditor/app/(site)/organizations/[organization_name]/settings/profile/page.tsx
💤 Files with no reviewable changes (1)
- editor/app/(api)/private/accounts/organizations/[org]/profile/route.ts
Comment on lines
+69
to
+72
| <OrganizationAvatarField | ||
| current_avatar_url={avatar_url} | ||
| display_name={data.display_name} | ||
| /> |
There was a problem hiding this comment.
display_name may be null, but the prop type requires string.
The Input for display name uses data.display_name ?? undefined (Line 87), implying the column is nullable, yet here it is passed into OrganizationAvatarField's display_name: string prop. The component already defends with display_name?.charAt(0), so widen the prop type to string | null | undefined (or coerce here) to keep the contract honest.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@editor/app/`(site)/organizations/[organization_name]/settings/profile/page.tsx
around lines 69 - 72, OrganizationAvatarField is being passed data.display_name
which can be null, but its display_name prop is typed as string; update the prop
contract or pass a safe value. Either change OrganizationAvatarField's prop type
for display_name to accept string | null | undefined (and keep its internal
checks like display_name?.charAt(0)), or coerce at the callsite by passing
display_name={data.display_name ?? undefined}. Update the TS type for
OrganizationAvatarField (and any related props/interfaces) or adjust this call
accordingly so types align.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
supabase/migrations/20260604120000_avatars_bucket_rls.sql (1)
35-85: ⚡ Quick winRLS lookup indexing for
public.rls_organization()
public.rls_organization(p_organization_id)checksorganization_memberwithom.organization_id = p_organization_idandom.user_id = auth.uid(). The migrations already add indexes onorganization_member(organization_id)andorganization_member(user_id). For best performance on this paired predicate, consider adding a composite index on(organization_id, user_id).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/migrations/20260604120000_avatars_bucket_rls.sql` around lines 35 - 85, The RLS helper public.rls_organization(p_organization_id) queries organization_member filtering by om.organization_id = p_organization_id and om.user_id = auth.uid(), so add a composite index to speed that paired lookup: create an index on organization_member(organization_id, user_id) in the migrations (alongside the existing single-column indexes) so public.rls_organization and the storage.objects RLS policies (which call public.rls_organization) use the composite index for efficient lookups.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@supabase/migrations/20260604120000_avatars_bucket_rls.sql`:
- Around line 3-20: The migration comment incorrectly states non-numeric
(user_profile/UUID) avatar objects “fall through” to other policies, but the
migration only creates the public SELECT and organization-scoped
INSERT/UPDATE/DELETE policies for bucket 'avatars' (scoped to the
`{organization_id}/avatar` path via the numeric-first-segment CASE and using the
public.rls_organization() helper), so either add explicit RLS policies to allow
UUID-keyed user_profile writes/removals to storage.objects where bucket_id =
'avatars' or update the comment to remove the claim; locate the migration SQL
that inserts policies for bucket 'avatars' and either (A) create matching
INSERT/UPDATE/DELETE policies that permit paths with non-numeric first segments
(e.g., UUIDs) for user_profile actors, or (B) change the descriptive text to
state that only org-scoped `{organization_id}/avatar` writes are permitted and
user_profile avatars are not covered.
---
Nitpick comments:
In `@supabase/migrations/20260604120000_avatars_bucket_rls.sql`:
- Around line 35-85: The RLS helper public.rls_organization(p_organization_id)
queries organization_member filtering by om.organization_id = p_organization_id
and om.user_id = auth.uid(), so add a composite index to speed that paired
lookup: create an index on organization_member(organization_id, user_id) in the
migrations (alongside the existing single-column indexes) so
public.rls_organization and the storage.objects RLS policies (which call
public.rls_organization) use the composite index for efficient lookups.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 60e7426d-e2fc-413d-995c-a7bcdc2dac6c
📒 Files selected for processing (3)
editor/app/(site)/organizations/[organization_name]/settings/profile/__tests__/actions.test.tseditor/app/(site)/organizations/[organization_name]/settings/profile/actions.tssupabase/migrations/20260604120000_avatars_bucket_rls.sql
Add an avatar control to the org General settings form. The profile submit
now goes through a "use server" action instead of the private API route:
- updateOrganizationProfile reads/updates the org row with the user-authed,
RLS-aware client (the organization SELECT/UPDATE membership policy is the
authorization gate); the avatar object write uses the service_role storage
client because the avatars bucket has no tracked storage RLS.
- avatar-field.tsx: pick + local preview + remove, carried as the avatar file
and a remove_avatar flag on the existing form.
- avatar stored at org-{id}/avatar (upsert); only avatar_path is persisted, the
URL is computed at read time via PublicUrls. png/jpeg/webp, <=2MB.
- remove the now-dead /private/accounts/organizations/[org]/profile route.
No new routes/pages; reuses the avatars bucket + existing render sites.
Membership gate, avatar type/size validation, org-{id}/avatar upsert upload,
and the remove (avatar_path: null) path. Mocks the supabase clients and
next/cache.
Per owner: server actions must not use service_role — RLS is the boundary
end to end. The avatar upload/remove now go through the same user-authed
createClient() as the org row read/update; authorization is enforced by
storage policies, not a code-side gate.
- New migration 20260604120000_avatars_bucket_rls.sql:
- idempotently creates the public 'avatars' bucket (no-op in prod, where it
already exists; on conflict do nothing) for local parity.
- adds org-membership storage.objects policies (insert/update/delete) scoped
via public.rls_organization on the path's first segment, mirroring the
storage/www bucket precedent. SELECT stays public for public-URL reads.
- each policy is drop-if-exists guarded to coexist with any same-named prod
policy; the membership check is CASE-guarded so non-numeric paths (the
bucket is shared with uuid-keyed user_profile avatars) return false instead
of raising on the ::bigint cast.
- Path scheme changed org-{id}/avatar -> {id}/avatar so the policy can parse
the org id via (storage.foldername(name))[1].
- Tests assert the upload/remove go through the user-auth client (no
service_role) and the {id}/avatar path.
- Reject missing display_name/email explicitly instead of persisting the literal string "null" via String(null). - Sniff png/jpeg/webp from the file's leading bytes rather than trusting the client-supplied File.type; reject mismatches and use the sniffed type as the upload contentType. - Extend tests: missing-required-field rejection, spoofed-MIME rejection, sniffed-type-wins, no-signature rejection.
The picked avatar File never reached the server action: the <form> lacked encType="multipart/form-data", and Next 16's default server-action bodySizeLimit (1MB) is below the 2MB avatar cap, 413-ing 1-2MB files before the action runs. - page.tsx: add encType="multipart/form-data" to the profile form. - next.config.ts: set experimental.serverActions.bodySizeLimit to '3mb' to fit the 2MB cap plus multipart overhead.
- Add encType="multipart/form-data" to the profile form so the picked avatar File is actually encoded into the server action request. - Revoke the createObjectURL preview on replace and unmount to stop the blob URL leak. - Widen display_name prop to string | null | undefined (it's nullable at the call site).
These are the only write policies on the avatars bucket. Under deny-by-default RLS, a non-numeric (e.g. uuid-keyed) path is simply denied for insert/update/delete — it does not fall through to another policy, because none exists. No user_profile avatar upload path exists in the codebase today.
softmarshmallow
force-pushed
the
feat/org-avatar-upload
branch
from
675cd40 to
db37bcd
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Lets an organization member set, replace, or remove their organization's avatar from the existing Organization → Settings → General form. No new routes or pages.
PRD (summary)
/organizations/[name]/settings/profile). Reuses the form's single Save.object-coverat render. Replace overwrites in place; Remove clears it.avatar_url— plus the new settings preview.Approach — server action + RLS end to end (no service_role)
The profile submit goes through a
"use server"action (actions.ts) instead of the old private API route. Every supabase call — the org row read/update and the avatar object upload/remove — runs through the same user-authed, RLS-awarecreateClient(). There is zeroservice_rolein this feature; RLS is the authorization boundary.updateOrganizationProfile(organization_name, formData)reads + updates theorganizationrow with the user-authed client (theorganizationSELECT/UPDATE membership policyrls_organizationis the gate; the org select is kept only for a friendly error).client.storage.from("avatars")— authorized by the new storage policies below.{organization_id}/avatar(folder-first so the policy can parse the org id),upsert: true. Onlyavatar_pathis persisted; the public URL is computed at read time viaPublicUrls.organization_avatar_url.revalidatePathon the settings page after the mutation.New migration —
supabase/migrations/20260604120000_avatars_bucket_rls.sqlavatarsbucket idempotently (insert … on conflict (id) do nothing, public) — a no-op in prod where it already exists; present for local-stack parity.storage.objectspolicies forbucket_id = 'avatars', scoped to org membership viapublic.rls_organization((storage.foldername(name))[1]::bigint)— mirroring the existingstorage/wwwbucket policy shape. Covers INSERT, UPDATE (upsert), DELETE (remove). SELECT stays public to match the public-URL read.No DB column migration:
organization.avatar_pathalready existed.Reconciliation notes (prod
avatarsbucket is untracked)on conflict do nothing.drop policy if exists …first, so the migration coexists with any same-named policy already on the prod bucket and is safe to (re)apply.user_profileavatars (uuid-keyed paths). A blanket::bigintcast would raise on those — so the membership check is wrapped in aCASE(guaranteed eval order): a non-numeric first segment yieldsfalseinstead of erroring, leaving those objects to whatever policy governs them.Files touched
…/settings/profile/actions.ts— the server action (user-auth client only).…/settings/profile/avatar-field.tsx— client control (pick + preview + remove).…/settings/profile/page.tsx— render current avatar, wire form to the bound action.editor/app/(api)/private/accounts/organizations/[org]/profile/route.ts— the now-dead route.supabase/migrations/20260604120000_avatars_bucket_rls.sql— bucket + RLS.…/settings/profile/__tests__/actions.test.ts— unit tests.Verification
pnpm lint(oxlint) — clean.pnpm turbo typecheck --filter=editor— 35/35 pass.pnpm vitest run …/actions.test.ts— 6/6 pass: membership gate, type + 2 MB validation, upload via the user-auth client to{id}/avatar(upsert), and remove (object delete +avatar_path: null). The tests assert noservice_rolepath is used.Local-env caveat (migration not live-applied)
The RLS policy could not be exercised against a live DB here: no Supabase CLI and Docker is unavailable, so
supabase db reset/ a real upload can't run. Proof is therefore: the migration SQL (modeled directly on the in-repostorage/wwwprecedents), the updated unit tests (which now assert the user-auth storage path), and typecheck. A reviewer with a local stack cansupabase db resetand exercise the round-trip end to end.Notes / follow-ups
upsertmeans the public URL is unchanged after a replace, so a cached avatar may need a hard refresh. Trivial cache-busting follow-up if desired.Summary by CodeRabbit
New Features
Tests
Chores